Skip to content

UI - Add storage name to delete primary/secondary storage dialog#5359

Merged
yadvr merged 3 commits into
apache:4.15from
utchoang:feature/add-storage-name-to-delete
Aug 24, 2021
Merged

UI - Add storage name to delete primary/secondary storage dialog#5359
yadvr merged 3 commits into
apache:4.15from
utchoang:feature/add-storage-name-to-delete

Conversation

@utchoang
Copy link
Copy Markdown

@utchoang utchoang commented Aug 24, 2021 •
edited
Loading

Description

Fixes #5344

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

image
image

How Has This Been Tested?

@utchoang
Copy link
Copy Markdown
Author

utchoang commented Aug 24, 2021

@blueorangutan ui

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Aug 24, 2021

@utchoang a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Aug 24, 2021

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/5359 (SL-JID-553)

@nvazquez nvazquez linked an issue Aug 24, 2021 that may be closed by this pull request
Add storage name to delete primary/secondary storage dialog #5344
Closed
nvazquez
nvazquez reviewed Aug 24, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@nvazquez nvazquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested the changes and works fine, thanks @utchoang

As an improvement, at least personally, I would prefer to see the pool name in the title and not as a field in the form. Something like:
image

@nvazquez nvazquez added this to the 4.15.2.0 milestone Aug 24, 2021
@utchoang
Copy link
Copy Markdown
Author

utchoang commented Aug 24, 2021 •
edited
Loading

@nvazquez The problem is that the case where the storage name is too long will be a break into a new line. Do you feel it's okay?
image

Option 2: I Can cut the text but the storage name will not be clear.
image

@davidjumani
Copy link
Copy Markdown
Contributor

davidjumani commented Aug 24, 2021

@utchoang If thats the case, can it be added in the warning message, with just the name in bold ?

@utchoang
Copy link
Copy Markdown
Author

utchoang commented Aug 24, 2021

@davidjumani Changed!
image
image

@utchoang
Copy link
Copy Markdown
Author

utchoang commented Aug 24, 2021

@blueorangutan ui

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Aug 24, 2021

@utchoang a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Aug 24, 2021

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/5359 (SL-JID-558)

@yadvr yadvr requested a review from nvazquez August 24, 2021 07:09
@yadvr
Copy link
Copy Markdown
Member

yadvr commented Aug 24, 2021

I think just displaying the name is enough, without the key "Name: "
@blueorangutan ui

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Aug 24, 2021

@rhtyd a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Aug 24, 2021

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/5359 (SL-JID-559)

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Aug 24, 2021

Tested OK - merging based on Nicolas and my LGTM.

@yadvr yadvr merged commit bc28ae4 into apache:4.15 Aug 24, 2021
@yadvr yadvr mentioned this pull request Aug 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr approved these changes

@nvazquez nvazquez Awaiting requested review from nvazquez

Assignees

@utchoang utchoang

Labels

component:UI

Projects

None yet

Milestone

4.15.2.0

Development

Successfully merging this pull request may close these issues.

Add storage name to delete primary/secondary storage dialog

5 participants

@utchoang @blueorangutan @davidjumani @yadvr @nvazquez